Skip to content

Refactor Entropy Calculations with Averaging Over Timesteps, Robust State Handling, and Enhanced Eigenvalue Filtering #130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
harryswift01 merged 18 commits into from
Aug 15, 2025
Merged

Refactor Entropy Calculations with Averaging Over Timesteps, Robust State Handling, and Enhanced Eigenvalue Filtering #130

harryswift01 merged 18 commits into from
Aug 15, 2025

Conversation

@harryswift01
Copy link
Contributor

@harryswift01 harryswift01 commented Aug 14, 2025 •
edited
Loading

Summary

This PR refactors and enhances the entropy computation to support averaging over timesteps, improve robustness against missing data, and streamline matrix operations. It also includes improvements to trajectory iteration, dihedral handling, and unit test coverage.

Changes

Implement Averaging Over Timesteps

  • Introduced running average logic for force and torque covariance matrices.
  • Updated build_covariance_matrices and update_force_torque_matrices to compute averages incrementally.
  • Removed post-loop summation and division, ensuring matrices are averaged during processing.

Improve Robustness in Entropy and Dihedral Computation

  • Refactored entropy computation to handle None and empty states safely.
  • Added type-safe checks for lists and NumPy arrays.
  • Skipped dihedral conformation assignment when no dihedrals are present.
  • Prevented unnecessary allocation and ensured entropy is only computed when valid states exist.

Refactor Trajectory Iteration

  • Replaced direct iteration with zip(index, frame) for consistent indexing.
  • Ensured final frame is not skipped during iteration.

Eigenvalue Filtering Enhancements

  • Filtered out complex and non-positive eigenvalues.
  • Converted valid values to real numbers and logged discarded ones.
  • Optimised filtering with a single-pass mask and summary warning.

Unit Test Improvements

  • Updated unit tests to reflect refactored logic.
  • Tidied up comments for clarity and maintainability.

Default Grouping Configuration

  • Changed default grouping from each to molecules for more intuitive behavior.

Impact

  • Improves numerical stability and correctness of entropy calculations.
  • Reduces memory usage by avoiding full-frame storage.
  • Enhances code readability and maintainability.
  • Provides more intuitive default behavior for users.

@harryswift01
Implement running average for force/torque covariance matrices:
06a36f0 
- Replaced per-frame overwrite and post-loop sum/division with incremental running average updates, matching the approach used in `waterEntropy`.
- Added `frame_counts` tracking to compute averages on the fly without storing all frame data.
- Updated `build_covariance_matrices` and `update_force_torque_matrices` to initialize averages on first frame and update them in place each step.
- Ensures final returned matrices are already averaged and representative of all processed timesteps.
@harryswift01
Refactor matrix averaging logic for modularity:
1497258 
- Removed division by `n_frames` in `entropy.py` to retain running averages
- Replaced `+=` with `=` in `get_matrices` to avoid accumulation across frames
- Simplified force and torque matrix updates in `levels.py` for consistency
@harryswift01
refactor: warn on negative eigenvalues instead of halting with error
d3c85d0
@harryswift01
Filter out the negative eigenvalues in the frequencies calculations a…
b516673 
…nd set them to 0
@harryswift01
Filter and clean eigenvalues: retain only real, strictly positive val…
2e8d34f 
…ues:

- Added logging for complex and non-positive eigenvalues with index and value
- Applied mask to filter out invalid values using np.isreal, np.isclose, and positivity check
- Converted filtered values to pure real numbers using .real
@harryswift01
Optimise eigenvalue filtering: single-pass mask with summary warning:
81bc1ef 
- Replaced per-element logging with a single summary warning based on mask length
- Improved performance and readability by avoiding redundant loops
@harryswift01
Merge branch 'main' of https://github.com/CCPBioSim/CodeEntropy into …
6c20773 
…127-add-averaging-over-timesteps
@harryswift01
Refactor trajectory iteration to use explicit index pairing via zip():
4eb9e94 
- Replaced direct iteration over trajectory slices with `zip(index, frame)`
- Ensures consistent indexing and avoids skipping final frame
@harryswift01
Refactor trajectory iteration to use explicit index pairing via zip()…
8575847 
… for `assign_conformation`:

- Replaced direct iteration over trajectory slices with `zip(index, frame)`
- Ensures consistent indexing and avoids skipping final frame
@harryswift01
Refactor dihedral computation to skip conformation assignment when no…
65237a6 
… dihedrals are present:

- Added check to avoid calling `assign_conformation` if `get_dihedrals` returns an empty list
- Prevented unnecessary allocation of empty `conformation` arrays
- Ensured `states` are only generated when valid dihedrals exist
- `conformational_entropy_calculation` is only called if the `state` is not empty
@harryswift01
Merge branch 'main' of https://github.com/CCPBioSim/CodeEntropy into …
ff1df47 
…127-add-averaging-over-timesteps
@harryswift01
Refactor entropy computation to handle missing and empty state data s…
206a3c3 
…afely:

- Replaced ambiguous truth checks with type-safe logic for lists and NumPy arrays
- Added guard against NoneType access for `states[group_id]` to prevent iteration errors
@harryswift01
fix and update unit tests to reflect changes made within this refactor
9d0c8a3
@harryswift01
tidy up comments within the unit tests
5863bdf
@harryswift01
change the default grouping configuration to group by molecules rat…
927d1aa 
…her than `each`
@harryswift01 harryswift01 added this to the 1.0.0 release milestone Aug 14, 2025
@harryswift01 harryswift01 self-assigned this Aug 14, 2025
@harryswift01 harryswift01 linked an issue Aug 14, 2025 that may be closed by this pull request
Add averaging of force and torque matrices over timesteps #127
Closed
3 tasks
@harryswift01 harryswift01 added the feature request New feature or request label Aug 14, 2025
jimboid
jimboid approved these changes Aug 14, 2025
View reviewed changes
Copy link
Member

@jimboid jimboid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has had a lot of eyeballing from the whole team whilst it has been being developed. Test data suggests the code changes here are good. A very hard fought addition from the team here, well done all!

jkalayan
jkalayan approved these changes Aug 14, 2025
View reviewed changes
Copy link
Collaborator

@jkalayan jkalayan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look great, looks like we have a working version of CodeEntropy :)

@skfegan
Copy link
Member

skfegan commented Aug 15, 2025

In entropy.py._process_conformational_entropy line 432 is:

group_states = states[group_id] if group_id < len(states) else None

I am not sure this is correct. In the grouping_molecules function (in group_molecules.py), the groups are dictionaries with the keys as the molecule number of the first molecule of that type. This means that the group_ids are all integers, but may not be consecutive. For grouping == "each" or most of our current examples it probably works, but if there are systems with multiples of molecules depending on how they are ordered in the topology weird things might happen if you try using the group_id as a counter rather than treating it as a label.

@harryswift01
Copy link
Contributor Author

harryswift01 commented Aug 15, 2025

In entropy.py._process_conformational_entropy line 432 is:

group_states = states[group_id] if group_id < len(states) else None

I am not sure this is correct. In the grouping_molecules function (in group_molecules.py), the groups are dictionaries with the keys as the molecule number of the first molecule of that type. This means that the group_ids are all integers, but may not be consecutive. For grouping == "each" or most of our current examples it probably works, but if there are systems with multiples of molecules depending on how they are ordered in the topology weird things might happen if you try using the group_id as a counter rather than treating it as a label.

This was an issue relating the the -1 input of end, we have now ensured that if the input of -1 is used we correctly assign end as the length of the Universe trajectory, this will resolve the indexing issues we have previously highlighted.

@harryswift01 harryswift01 merged commit bc10e47 into main Aug 15, 2025
7 checks passed
@harryswift01 harryswift01 deleted the 127-add-averaging-over-timesteps branch August 15, 2025 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimboid jimboid jimboid approved these changes

@jkalayan jkalayan jkalayan approved these changes

@skfegan skfegan Awaiting requested review from skfegan

Assignees

@harryswift01 harryswift01

Labels

feature request New feature or request

Projects

None yet

Milestone

1.0.0 release

Development

Successfully merging this pull request may close these issues.

Add averaging of force and torque matrices over timesteps

5 participants

@harryswift01 @skfegan @jimboid @jkalayan